Support -mnop-mcount and -mrecord-mcount on x86_64 elf targets.#184641
Support -mnop-mcount and -mrecord-mcount on x86_64 elf targets.#184641
Conversation
Support mnop-mcount and mrecord-mcount attributes when used with the fentry-call=true attribute. Similar to SystemZ, use the mnop-mcount attribute to place a nop instead of a call to __fentry__. Likewise, use the mrecord-mcount attribute to record the location of the call to fentry (or nop if used with the above) to a section named __mcount_loc.
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-backend-x86 Author: Paul Murphy (pmur) ChangesI found out these attributes are only support on s390x when prototyping my Rust rfc for expanded instrumented function support. Also, is this the correct/preferred way to insert nop's on x86? Full diff: https://github.com/llvm/llvm-project/pull/184641.diff 10 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index e860136aae5b3..2807be2c7edd0 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -6884,14 +6884,16 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
<< A->getAsString(Args) << TripleStr;
}
if (Arg *A = Args.getLastArg(options::OPT_mnop_mcount)) {
- if (Arch == llvm::Triple::systemz)
+ if (Arch == llvm::Triple::systemz || (TC.getTriple().isX86_64() &&
+ TC.getTriple().isOSBinFormatELF()))
A->render(Args, CmdArgs);
else
D.Diag(diag::err_drv_unsupported_opt_for_target)
<< A->getAsString(Args) << TripleStr;
}
if (Arg *A = Args.getLastArg(options::OPT_mrecord_mcount)) {
- if (Arch == llvm::Triple::systemz)
+ if (Arch == llvm::Triple::systemz || (TC.getTriple().isX86_64() &&
+ TC.getTriple().isOSBinFormatELF()))
A->render(Args, CmdArgs);
else
D.Diag(diag::err_drv_unsupported_opt_for_target)
diff --git a/clang/test/CodeGen/mnop-mcount.c b/clang/test/CodeGen/mnop-mcount.c
index ebb21cc560e37..31b4e995cff02 100644
--- a/clang/test/CodeGen/mnop-mcount.c
+++ b/clang/test/CodeGen/mnop-mcount.c
@@ -6,6 +6,14 @@
// RUN: %s 2>&1 | FileCheck -check-prefix=NOPG %s
// RUN: %clang_cc1 -mnop-mcount -triple s390x-ibm-linux -emit-llvm -o - %s \
// RUN: 2>&1 | FileCheck -check-prefix=NOPG %s
+// RUN: %clang_cc1 -pg -mfentry -mnop-mcount -triple x86_64-ibm-linux -emit-llvm \
+// RUN: -o - %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -pg -mnop-mcount -triple x86_64-ibm-linux -emit-llvm -o - \
+// RUN: %s 2>&1 | FileCheck -check-prefix=NOMFENTRY %s
+// RUN: %clang_cc1 -mfentry -mnop-mcount -triple x86_64-ibm-linux -emit-llvm -o - \
+// RUN: %s 2>&1 | FileCheck -check-prefix=NOPG %s
+// RUN: %clang_cc1 -mnop-mcount -triple x86_64-ibm-linux -emit-llvm -o - %s \
+// RUN: 2>&1 | FileCheck -check-prefix=NOPG %s
int foo(void) {
return 0;
diff --git a/clang/test/CodeGen/mrecord-mcount.c b/clang/test/CodeGen/mrecord-mcount.c
index 292a807073cef..832d4ee5e3e9f 100644
--- a/clang/test/CodeGen/mrecord-mcount.c
+++ b/clang/test/CodeGen/mrecord-mcount.c
@@ -6,6 +6,14 @@
// RUN: %s 2>&1 | FileCheck -check-prefix=NOPG %s
// RUN: %clang_cc1 -mrecord-mcount -triple s390x-ibm-linux -emit-llvm -o - %s \
// RUN: 2>&1 | FileCheck -check-prefix=NOPG %s
+// RUN: %clang_cc1 -pg -mfentry -mrecord-mcount -triple x86_64-ibm-linux -emit-llvm \
+// RUN: -o - %s 2>&1 | FileCheck %s
+// RUN: not %clang_cc1 -pg -mrecord-mcount -triple x86_64-ibm-linux -emit-llvm -o - \
+// RUN: %s 2>&1 | FileCheck -check-prefix=NOMFENTRY %s
+// RUN: %clang_cc1 -mfentry -mrecord-mcount -triple x86_64-ibm-linux -emit-llvm -o - \
+// RUN: %s 2>&1 | FileCheck -check-prefix=NOPG %s
+// RUN: %clang_cc1 -mrecord-mcount -triple x86_64-ibm-linux -emit-llvm -o - %s \
+// RUN: 2>&1 | FileCheck -check-prefix=NOPG %s
int foo(void) {
return 0;
diff --git a/clang/test/Driver/mcount.c b/clang/test/Driver/mcount.c
index 70f554745b9c3..72d48b3806fe7 100644
--- a/clang/test/Driver/mcount.c
+++ b/clang/test/Driver/mcount.c
@@ -1,12 +1,10 @@
// RUN: %clang --target=s390x -c -### %s -mnop-mcount -mrecord-mcount 2>&1 | FileCheck %s
+// RUN: %clang --target=x86_64 -c -### %s -mnop-mcount -mrecord-mcount 2>&1 | FileCheck %s
// CHECK: "-mnop-mcount"
// CHECK: "-mrecord-mcount"
-// RUN: not %clang --target=x86_64 -c -### %s -mnop-mcount -mrecord-mcount 2>&1 | FileCheck --check-prefix=ERR1 %s
// RUN: not %clang --target=aarch64 -c -### %s -mnop-mcount -mrecord-mcount 2>&1 | FileCheck --check-prefix=ERR2 %s
-// ERR1: error: unsupported option '-mnop-mcount' for target 'x86_64'
-// ERR1: error: unsupported option '-mrecord-mcount' for target 'x86_64'
// ERR2: error: unsupported option '-mnop-mcount' for target 'aarch64'
// ERR2: error: unsupported option '-mrecord-mcount' for target 'aarch64'
diff --git a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
index 101ea3e231a5c..aa7efb8a05e1c 100644
--- a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -179,11 +179,19 @@ namespace {
bool runOnMachineFunction(MachineFunction &MF) override {
// Reset the subtarget each time through.
Subtarget = &MF.getSubtarget<X86Subtarget>();
- IndirectTlsSegRefs = MF.getFunction().hasFnAttribute(
- "indirect-tls-seg-refs");
+ const Function &F = MF.getFunction();
+
+ IndirectTlsSegRefs = F.hasFnAttribute("indirect-tls-seg-refs");
+
+ if (F.getFnAttribute("fentry-call").getValueAsString() != "true") {
+ if (F.hasFnAttribute("mnop-mcount"))
+ report_fatal_error("mnop-mcount only supported with fentry-call");
+ if (F.hasFnAttribute("mrecord-mcount"))
+ report_fatal_error("mrecord-mcount only supported with fentry-call");
+ }
// OptFor[Min]Size are used in pattern predicates that isel is matching.
- OptForMinSize = MF.getFunction().hasMinSize();
+ OptForMinSize = F.hasMinSize();
return SelectionDAGISel::runOnMachineFunction(MF);
}
diff --git a/llvm/lib/Target/X86/X86MCInstLower.cpp b/llvm/lib/Target/X86/X86MCInstLower.cpp
index 4ef4718e1b01c..8bbd40d973fab 100644
--- a/llvm/lib/Target/X86/X86MCInstLower.cpp
+++ b/llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -26,6 +26,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/BinaryFormat/ELF.h"
#include "llvm/CodeGen/MachineBranchProbabilityInfo.h"
#include "llvm/CodeGen/MachineConstantPool.h"
#include "llvm/CodeGen/MachineFunction.h"
@@ -44,6 +45,7 @@
#include "llvm/MC/MCInst.h"
#include "llvm/MC/MCInstBuilder.h"
#include "llvm/MC/MCSection.h"
+#include "llvm/MC/MCSectionELF.h"
#include "llvm/MC/MCStreamer.h"
#include "llvm/MC/MCSymbol.h"
#include "llvm/MC/TargetRegistry.h"
@@ -885,6 +887,22 @@ void X86AsmPrinter::LowerFENTRY_CALL(const MachineInstr &MI,
X86MCInstLower &MCIL) {
bool Is64Bits = Subtarget->is64Bit();
MCContext &Ctx = OutStreamer->getContext();
+
+ if (MF->getFunction().hasFnAttribute("mrecord-mcount")) {
+ MCSymbol *DotSym = OutContext.createTempSymbol();
+ OutStreamer->pushSection();
+ OutStreamer->switchSection(
+ Ctx.getELFSection("__mcount_loc", ELF::SHT_PROGBITS, ELF::SHF_ALLOC));
+ OutStreamer->emitSymbolValue(DotSym, Is64Bits ? 8 : 4);
+ OutStreamer->popSection();
+ OutStreamer->emitLabel(DotSym);
+ }
+
+ if (MF->getFunction().hasFnAttribute("mnop-mcount")) {
+ emitX86Nops(*OutStreamer, 5, Subtarget);
+ return;
+ }
+
MCSymbol *fentry = Ctx.getOrCreateSymbol("__fentry__");
const MCSymbolRefExpr *Op = MCSymbolRefExpr::create(fentry, Ctx);
diff --git a/llvm/test/CodeGen/X86/mnop-mcount-01.ll b/llvm/test/CodeGen/X86/mnop-mcount-01.ll
new file mode 100644
index 0000000000000..ddefa497659f3
--- /dev/null
+++ b/llvm/test/CodeGen/X86/mnop-mcount-01.ll
@@ -0,0 +1,26 @@
+; RUN: llc %s -mtriple=x86_64-linux-gnu -o - -verify-machineinstrs \
+; RUN: | FileCheck %s
+
+define void @test1() #0 {
+entry:
+ ret void
+
+; CHECK-LABEL: @test1
+; CHECK: callq __fentry__
+; CHECK-NOT: nopl 8(%rax,%rax)
+; CHECK: retq
+}
+
+define void @test2() #1 {
+entry:
+ ret void
+
+; CHECK-LABEL: @test2
+; CHECK-NOT: callq __fentry__
+; CHECK: nopl 8(%rax,%rax)
+; CHECK: retq
+}
+
+attributes #0 = { "fentry-call"="true" }
+attributes #1 = { "fentry-call"="true" "mnop-mcount" }
+
diff --git a/llvm/test/CodeGen/X86/mnop-mcount-02.ll b/llvm/test/CodeGen/X86/mnop-mcount-02.ll
new file mode 100644
index 0000000000000..d576e0f289818
--- /dev/null
+++ b/llvm/test/CodeGen/X86/mnop-mcount-02.ll
@@ -0,0 +1,10 @@
+; RUN: not --crash llc %s -mtriple=x86_64-linux-gnu -o - 2>&1 | FileCheck %s
+;
+; CHECK: LLVM ERROR: mnop-mcount only supported with fentry-call
+
+define void @test1() #0 {
+entry:
+ ret void
+}
+
+attributes #0 = { "instrument-function-entry-inlined"="mcount" "mnop-mcount" }
diff --git a/llvm/test/CodeGen/X86/mrecord-mcount-01.ll b/llvm/test/CodeGen/X86/mrecord-mcount-01.ll
new file mode 100644
index 0000000000000..6202a652b8831
--- /dev/null
+++ b/llvm/test/CodeGen/X86/mrecord-mcount-01.ll
@@ -0,0 +1,32 @@
+; RUN: llc %s -mtriple=x86_64-linux-gnu -o - -verify-machineinstrs \
+; RUN: | FileCheck %s
+
+define void @test1() #0 {
+entry:
+ ret void
+
+; CHECK-LABEL: test1:
+; CHECK: .section __mcount_loc,"a",@progbits
+; CHECK: .quad .Ltmp0
+; CHECK: .text
+; CHECK: .Ltmp0:
+; CHECK: callq __fentry__
+; CHECK: retq
+}
+
+define void @test2() #1 {
+entry:
+ ret void
+
+; CHECK-LABEL: test2:
+; CHECK: .section __mcount_loc,"a",@progbits
+; CHECK: .quad .Ltmp1
+; CHECK: .text
+; CHECK: .Ltmp1:
+; CHECK: nopl 8(%rax,%rax)
+; CHECK-NOT: callq __fentry__
+; CHECK: retq
+}
+
+attributes #0 = { "fentry-call"="true" "mrecord-mcount" }
+attributes #1 = { "fentry-call"="true" "mnop-mcount" "mrecord-mcount" }
diff --git a/llvm/test/CodeGen/X86/mrecord-mcount-02.ll b/llvm/test/CodeGen/X86/mrecord-mcount-02.ll
new file mode 100644
index 0000000000000..a1f80f1840c41
--- /dev/null
+++ b/llvm/test/CodeGen/X86/mrecord-mcount-02.ll
@@ -0,0 +1,10 @@
+; RUN: not --crash llc %s -mtriple=x86_64-linux-gnu -o - 2>&1 | FileCheck %s
+;
+; CHECK: LLVM ERROR: mrecord-mcount only supported with fentry-call
+
+define void @test1() #0 {
+entry:
+ ret void
+}
+
+attributes #0 = { "instrument-function-entry-inlined"="mcount" "mrecord-mcount" }
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
These are gated by -mfentry.
0c14b0a to
27616c7
Compare
nikic
left a comment
There was a problem hiding this comment.
How does this differ from the "patchable-function-entry" attribute? I believe that one has good cross target support for inserting function entry nops on at least X86, AArch64, PowerPC, RISCV, LoongArch and SystemZ.
|
Added some X86 reviewers, but I think this could use more context on why we want to support these options, and why we want to support them on s390x and x86 in particular. Is it just because those are the two arches where GCC supports them? |
I suppose fentry is a more specialized variant. Interestingly, the x86-64 kernel uses both. It seems to be a crude way to separate bpf and ftrace usage of patchable entries. Of minor note, the gcc x86-64 windows targets default to using fentry over mcount when using |
I found out these attributes are only support on s390x when prototyping my Rust rfc for expanded instrumented function support.
Also, is this the correct/preferred way to insert nop's on x86?